fix(ai-agents): preserve service fields during container defaults#9211
Open
glharper wants to merge 6 commits into
Open
fix(ai-agents): preserve service fields during container defaults#9211glharper wants to merge 6 commits into
glharper wants to merge 6 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fed9e97b-e79b-4889-ac76-0d9a428599cd
glharper
requested review from
JeffreyCA,
huimiu,
hund030,
therealjohn,
trangevi and
trrwilson
as code owners
July 20, 2026 13:52
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Preserves user-authored agent service fields while persisting resolved container defaults.
Changes:
- Replaces full service updates with targeted container updates.
- Supports inline and legacy configuration layouts.
- Adds regression coverage for preserved fields and image templates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/cmd/listen.go |
Persists only container settings. |
internal/cmd/listen_test.go |
Tests both configuration layouts and preservation behavior. |
jongio
approved these changes
Jul 20, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Targeted SetServiceConfigValue on just the container path is the right fix for #9152 - it keeps hooks and ${ENV} image templating that the old full-block AddService was dropping. One non-blocking note inline about the duplicated inline-vs-legacy detection.
trangevi
reviewed
Jul 20, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fed9e97b-e79b-4889-ac76-0d9a428599cd
| if _, err := azdClient.Project().AddService(ctx, req); err != nil { | ||
| return fmt.Errorf("adding agent service to project: %w", err) | ||
| if _, err := azdClient.Project().SetServiceConfigValue(ctx, &azdext.SetServiceConfigValueRequest{ | ||
| ServiceName: svc.GetName(), |
|
|
||
| if _, err := azdClient.Project().AddService(ctx, req); err != nil { | ||
| return fmt.Errorf("adding agent service to project: %w", err) | ||
| if _, err := azdClient.Project().SetServiceConfigValue(ctx, &azdext.SetServiceConfigValueRequest{ |
jongio
approved these changes
Jul 20, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
The refactor addresses my earlier note: the inline-vs-legacy selection now lives entirely in SetAgentContainerSettings, which returns the dotted path and value, so the listener no longer re-derives the shape or reads the value back. New test covers both the inline and legacy layouts. Wiring and error paths look correct.
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
Copilot
AI
requested review from
RickWinter,
hemarina,
richardpark-msft,
tg-msft and
vhvb1989
as code owners
July 21, 2026 20:53
Comment on lines
+750
to
+754
| services, ok := cfg.Raw()["services"].(map[string]any) | ||
| if !ok { | ||
| return nil, fmt.Errorf("services configuration not found") | ||
| } | ||
| serviceConfig, ok := services[req.ServiceName].(map[string]any) |
| return &azdext.EmptyResponse{}, nil | ||
| } | ||
|
|
||
| func TestPopulateContainerSettings_UsesTargetedConfigUpdate(t *testing.T) { |
jongio
approved these changes
Jul 21, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Re-reviewed the follow-up commit. The mutex serializes concurrent config mutations on the shared project service, and switching SetServiceConfigValue to write into the resolved service sub-config correctly handles dotted service names. Verified the sub-config mutation propagates back before save, and there's no reentrant locking. Good coverage with the new dotted-name and concurrency tests.
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
Contributor
Resolved and pushed in merge commit |
Comment on lines
559
to
562
| containerPath := "container" | ||
| props := svc.GetAdditionalProperties() | ||
| if props == nil || len(props.GetFields()) == 0 { | ||
| if cfg := svc.GetConfig(); cfg != nil && len(cfg.GetFields()) > 0 { |
jongio
approved these changes
Jul 22, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Re-checked after the merge with main and the container-settings conflict resolution. The net PR change is unchanged from my earlier review: the targeted SetServiceConfigValue writes into the resolved service sub-config (so dotted service names resolve correctly), SetAgentContainerSettings returns the path and value the listener persists without re-deriving the shape, and configMutationMu serializes concurrent config mutations. Tests for the inline and legacy layouts plus the dotted-name and concurrency cases are intact. CI is green.
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
Comment on lines
+2655
to
+2659
| svc := newProjectServiceWithYaml(t, `name: test-project | ||
| services: | ||
| my.agent: | ||
| host: appservice | ||
| `) |
Contributor
There was a problem hiding this comment.
Added the host-level serialization regression test in d08d5b8. It updates container settings through SetServiceConfigValue, reloads the saved YAML, and verifies the service hooks and ${MY_TAG} image remain unchanged.
jongio
approved these changes
Jul 23, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Re-reviewed the follow-up commit. The container write now routes through ServiceConfigProps, so a legacy azure.ai.agent service that carries unrelated inline keys (no kind) still persists under config.container instead of landing in AdditionalProperties. The persist is also skipped when the service uses a root $ref, which avoids writing the resolved cpu/memory back as an inline container sibling next to the reference and defeating the file include. New tests cover both paths: the unrelated-inline legacy case and the no-persist-on-file-ref case. Targeted internal/cmd and internal/project tests pass locally.
Co-authored-by: glharper <64209257+glharper@users.noreply.github.com>
jongio
approved these changes
Jul 23, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Re-reviewed the follow-up commit. It adds a host-level regression test that drives a container.resources.cpu update through SetServiceConfigValue on an azure.ai.agent service, then reloads the saved config and asserts both the templated image and the predeploy/postdeploy hooks survive. That locks in the exact behavior this PR fixes for #9152. Test-only delta, no production changes, and it follows the parallel/t.Context patterns used by the neighboring cases.
Contributor
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
azure.yamlTesting
go test ./internal/cmd -count=1go build ./...golangci-lint run ./internal/cmd/...cspell lint internal/cmd/listen.go internal/cmd/listen_test.goFixes #9152